Validate funding contributions reserves in `splice_init` and `splice_ack` handling #4011

tankyleo · 2025-08-14T01:21:41Z

From BOLT 2:

- If `funding_contribution_satoshis` is negative and its absolute value
  is greater than the sending node's current channel balance:
  - MUST send a `warning` and close the connection or send an `error`
    and fail the channel.

We allow the remote to be below the new reserve as long as their
funding contribution is not negative; we don't care whether they were
above or below the previous funding reserve.

ldk-reviews-bot · 2025-08-14T01:21:43Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-08-14T01:31:40Z

Codecov Report

❌ Patch coverage is 71.73913% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.73%. Comparing base (e664b7e) to head (9f4dc46).
⚠️ Report is 71 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/sign/tx_builder.rs	38.09%	13 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4011      +/-   ##
==========================================
- Coverage   88.74%   88.73%   -0.02%     
==========================================
  Files         176      176              
  Lines      128638   128857     +219     
  Branches   128638   128857     +219     
==========================================
+ Hits       114164   114339     +175     
- Misses      11877    11922      +45     
+ Partials     2597     2596       -1

Flag	Coverage Δ
fuzzing	`21.92% <17.39%> (+0.08%)`	⬆️
tests	`88.56% <71.73%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ldk-reviews-bot · 2025-08-16T01:25:45Z

🔔 1st Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-08-18T01:26:28Z

🔔 2nd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tankyleo · 2025-08-19T18:02:47Z

On hold until splice-out PR gets in.

ldk-reviews-bot · 2025-08-20T01:27:13Z

🔔 3rd Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

lightning/src/ln/channel.rs

wpaulino · 2025-08-26T22:44:44Z

lightning/src/sign/tx_builder.rs

+impl NextCommitmentStats {
+	pub(crate) fn get_balances_including_fee_msat(&self) -> (Option<u64>, Option<u64>) {
+		let holder_balance_incl_fee_msat = if self.is_outbound_from_holder {
+			self.holder_balance_before_fee_msat


Does this already have the anchor outputs value subtracted?

lightning/src/sign/tx_builder.rs

lightning/src/ln/channel.rs

wpaulino · 2025-08-26T23:28:41Z

lightning/src/ln/channel.rs

-
-		// TODO(splicing): Pre-check for reserve requirement
-		// (Note: It should also be checked later at tx_complete)
+		if their_funding_contribution.is_negative() {


There's also the case where we make a positive contribution and the counterparty does too, but a much larger one, bringing the reserve high enough that even after our contribution we still are not able to meet it.

From my reading of the spec, seems a party should only complain in cases where the counterparty's funding contribution is negative.

Did we want to be stricter than the spec here ?

This is in the spec actually: https://github.com/lightning/bolts/pull/1160/files#diff-ed04ca2c673fd6aabde69389511fa9ee60cb44d6b2ef6c88b549ffaa753d6afeR1792

lightning/src/ln/channel.rs

wpaulino · 2025-08-26T23:34:30Z

lightning/src/ln/channel.rs

+
+		// Reserve check on local commitment transaction
+
+		let splice_local_commitment_stats = self.context.get_next_local_commitment_stats(


The "next" naming is odd here because we're actually building an alternative version of the current commitment transaction

Agreed :) If a squint a little, the "next" commitment transaction will have different to_local, to_remote balances, that's the one we are validating here. As far as I see at this point, this "alternative" commitment transaction is the one we will sign "next" ?

This is from 3921 feel free to take a look.

ldk-reviews-bot · 2025-08-26T23:35:08Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

tankyleo · 2025-08-27T02:30:54Z

Rebase on merge-base (diff):

Always run the reserve check, not just when the funding contribution is negative.
Make sure that the funder of the channel can pay for an additional nondust HTLC after the contributions are applied.
Emit WarnAndDisconnect if the balance is exhausted.
Style improvements.

tankyleo · 2025-08-27T05:54:02Z

Amend: (diff)

Run the reserve check anytime the funding contribution is not zero.

lightning/src/ln/channel.rs

tankyleo · 2025-08-27T16:27:00Z

Rebasing now to fix conflict...

lightning/src/ln/channel.rs

As much as possible, we want to only mutate state once we are done with input validation. This also removes complaints when helper functions during validation take a `&self`.

As in `splice_init`, this helps clearly delineate `splice_ack` message validation from the subsequent state mutations. This is a code-move.

We will validate the reserve requirements on the new `FundingScope` in `validate_splice_contribution`.

`NextCommitmentStats` provides the commitment transaction fee as a separate value to assist with applying a multiplier on it in `can_accept_incoming_htlc`. Nonetheless in most cases, we want the balances to include the commitment transaction fee, so here we add a helper that gives us these balances.

Makes it consistent with `get_balances_including_fee`, itself added in the previous commit.

This applies to both `splice_init` and `splice_ack` messages. From BOLT 2: ``` - If `funding_contribution_satoshis` is negative and its absolute value is greater than the sending node's current channel balance: - MUST send a `warning` and close the connection or send an `error` and fail the channel. ``` Further down also: ``` If a side does not meet the reserve requirements, that's OK: but if they take funds out of the channel, they must ensure that they do meet them. If your peer adds a massive amount to the channel, then you only have to add more reserve if you want to contribute to the splice (and you can use `tx_remove_output` and/or `tx_remove_input` part-way through if this happens). ``` Therefore, we run the reserve check anytime the contribution is not equal to zero.

tankyleo · 2025-08-27T17:03:11Z

Pushed from the wrong machine, we should be good now, see this aggregate diff, , rebased on d55b4f8

TheBlueMatt · 2025-08-28T21:17:47Z

lightning/src/sign/tx_builder.rs

@@ -48,6 +49,26 @@ pub(crate) struct NextCommitmentStats {
 	pub extra_nondust_htlc_on_counterparty_tx_dust_exposure_msat: Option<u64>,
 }

+impl NextCommitmentStats {
+	pub(crate) fn get_balances_including_fee_msat(&self) -> (Option<u64>, Option<u64>) {


nit: for methods like this I really prefer we name them something that includes the order of the returned values. That way the callsite reads let (holder, counterparty) = x.get_holder_counterparty_balances_incl_fee_msat() and it becaomes obvious if you invert the order. Alternatively, we can return Yet Another Struct.

Thanks will put this in #4032 likely

wpaulino · 2025-08-28T22:06:36Z

lightning/src/ln/channel.rs

-		// TODO(splicing): Pre-check for reserve requirement
-		// (Note: It should also be checked later at tx_complete)
+		if their_funding_contribution != SignedAmount::ZERO {
+			self.validate_their_funding_contribution_reserve(&splice_funding)?;


I think we're still missing validation for when we're the initiator and we contribute but the counterparty doesn't. Ideally the counterparty does reject a contribution we propose that would put us under the reserve, but we shouldn't be sending them invalid things in the first place.

I think we're still missing validation for when we're the initiator and we contribute but the counterparty doesn't. Ideally the counterparty does reject a contribution we propose that would put us under the reserve, but we shouldn't be sending them invalid things in the first place.

Yes thank you, I'm planning to add a check in fn splice_channel for the case where we are the initiator.

Did you have in mind to add a similar check on our own funding contribution in fn splice_init in case we are not the initator ?

That should already be covered by validate_splice_init? They must have something to contribute if they're the initiator (it doesn't look like we enforce this though we should), so we'd end up calling validate_their_funding_contribution_reserve.

That should already be covered by validate_splice_init? They must have something to contribute if they're the initiator (it doesn't look like we enforce this though we should), so we'd end up calling validate_their_funding_contribution_reserve.

Right yes sounds like a quick check that splice_init.funding_contribution is not zero on the receiver side of this message.

On the sending side, I'm thinking we'll also want to double check our own funding contribution before sending splice init.

wpaulino · 2025-08-28T22:32:55Z

lightning/src/ln/channel.rs

+
+	/// Used to validate a negative `funding_contribution_satoshis` in `splice_init` and `splice_ack` messages.
+	#[cfg(splicing)]
+	fn validate_their_funding_contribution_reserve(


Maybe rename this since we're checking the reserve on both sides (remote balance on both local and remote commitments). Comment could use an update too.

Before sending a `splice_init`: * check that we can afford any negative contributions given our *current* channel balance. * check that we meet the v2 reserve requirement once our contribution is applied to the *next* funding scope.

tankyleo changed the title ~~Validate negative funding contributions in splice_ack and splice_init messages~~ Validate negative funding contributions in splice_init and splice_ack messages Aug 14, 2025

tankyleo force-pushed the splice-reserve-check branch from 6d6b07c to 93965c6 Compare August 14, 2025 01:24

tankyleo requested a review from wpaulino August 14, 2025 01:25

tankyleo added this to Weekly Goals Aug 14, 2025

tankyleo self-assigned this Aug 14, 2025

wpaulino mentioned this pull request Aug 18, 2025

Add splice-out support #3979

Merged

tankyleo removed the request for review from wpaulino August 20, 2025 01:37

tankyleo marked this pull request as draft August 20, 2025 01:38

jkczyz mentioned this pull request Aug 21, 2025

Dual-funded channels and Splicing Project Tracking #1621

Open

7 tasks

tankyleo force-pushed the splice-reserve-check branch from 93965c6 to d0023e3 Compare August 26, 2025 22:14

tankyleo marked this pull request as ready for review August 26, 2025 22:14

ldk-reviews-bot requested a review from valentinewallace August 26, 2025 22:15

tankyleo requested review from wpaulino and removed request for valentinewallace August 26, 2025 22:15

wpaulino reviewed Aug 26, 2025

View reviewed changes

tankyleo force-pushed the splice-reserve-check branch from d0023e3 to 835b67b Compare August 27, 2025 02:26

tankyleo requested a review from wpaulino August 27, 2025 02:37

tankyleo changed the title ~~Validate negative funding contributions in splice_init and splice_ack messages~~ Validate funding contributions reserves in splice_init and splice_ack handling Aug 27, 2025

tankyleo force-pushed the splice-reserve-check branch from 835b67b to 453a211 Compare August 27, 2025 05:51

graphite-app bot reviewed Aug 27, 2025

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

tankyleo force-pushed the splice-reserve-check branch from 453a211 to 9798f7a Compare August 27, 2025 16:33

graphite-app bot reviewed Aug 27, 2025

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

tankyleo added 7 commits August 27, 2025 16:54

Finish validation in splice_ack before taking a &mut self

09926fe

As much as possible, we want to only mutate state once we are done with input validation. This also removes complaints when helper functions during validation take a `&self`.

Add validate_splice_ack helper function

6fd902a

As in `splice_init`, this helps clearly delineate `splice_ack` message validation from the subsequent state mutations. This is a code-move.

Make for_splice infallible

e55084c

Let validate_splice_contribution construct the new FundingScope

0aa19d9

We will validate the reserve requirements on the new `FundingScope` in `validate_splice_contribution`.

Cleanup the style of tx_builder::subtract_addl_outputs

79c2645

Makes it consistent with `get_balances_including_fee`, itself added in the previous commit.

tankyleo force-pushed the splice-reserve-check branch from 9798f7a to 7a56757 Compare August 27, 2025 16:55

tankyleo moved this to Goal: Merge in Weekly Goals Aug 28, 2025

tankyleo requested a review from TheBlueMatt August 28, 2025 17:13

TheBlueMatt previously approved these changes Aug 28, 2025

View reviewed changes

TheBlueMatt mentioned this pull request Aug 28, 2025

Remaining follow-ups from 3921 #4032

Open

wpaulino reviewed Aug 28, 2025

View reviewed changes

tankyleo dismissed TheBlueMatt’s stale review via 5b205ec August 29, 2025 07:22

tankyleo added 2 commits August 29, 2025 07:29

Enforce that the splice initiator set a non-zero funding contribution

9f4dc46

tankyleo force-pushed the splice-reserve-check branch from 5b205ec to 9f4dc46 Compare August 29, 2025 07:30

tankyleo requested review from wpaulino and TheBlueMatt August 29, 2025 07:55


		// Reserve check on local commitment transaction

		let splice_local_commitment_stats = self.context.get_next_local_commitment_stats(

Validate funding contributions reserves in splice_init and splice_ack handling #4011

Are you sure you want to change the base?

Validate funding contributions reserves in splice_init and splice_ack handling #4011

Conversation

tankyleo commented Aug 14, 2025

Uh oh!

ldk-reviews-bot commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ldk-reviews-bot commented Aug 16, 2025

Uh oh!

ldk-reviews-bot commented Aug 18, 2025

Uh oh!

tankyleo commented Aug 19, 2025

Uh oh!

ldk-reviews-bot commented Aug 20, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Aug 26, 2025

Uh oh!

tankyleo commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tankyleo commented Aug 27, 2025

Uh oh!

Uh oh!

tankyleo commented Aug 27, 2025

Uh oh!

Uh oh!

tankyleo commented Aug 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Validate funding contributions reserves in `splice_init` and `splice_ack` handling #4011

Validate funding contributions reserves in `splice_init` and `splice_ack` handling #4011

ldk-reviews-bot commented Aug 14, 2025 •

edited

Loading

codecov bot commented Aug 14, 2025 •

edited

Loading

tankyleo commented Aug 27, 2025 •

edited

Loading